Adding Kernel PCovC Code #254

rvasav26 · 2025-06-08T22:05:29Z

Adding Kernel PCovC code, examples, and testing suite. Adding a KPCov base class that both Kernel PCovR and Kernel PCovC inherit from.

Contributor (creator of PR) checklist

Tests updated (for new features and bugfixes)?
Documentation updated (for new features)?
Issue referenced (for PRs that solve an issue)?

For Reviewer

CHANGELOG updated if important change?

📚 Documentation preview 📚: https://scikit-matter--254.org.readthedocs.build/en/254/

rosecers

This looks pretty good. In general, I like to keep PR's single purpose (@ceriottm will tell you I can be quite vigilant). The changes to PCovC seem small enough, however. I'd just go through and finalize a couple reformatting things then we should be good.

rosecers · 2025-06-24T22:39:37Z

src/skmatter/decomposition/_kernel_pcovc.py

+        Number of components to keep.
+        if n_components is not set all components are kept::
+
+            n_components == n_samples


This should be inline

We could, but if we do, we would lose this appearance (and docs build formatting fails):

rosecers · 2025-06-24T22:40:57Z

src/skmatter/decomposition/_kernel_pcovc.py

+    classifier: `estimator object` or `precomputed`, default=None
+        classifier for computing :math:`{\mathbf{Z}}`. The classifier should be one of
+        `sklearn.linear_model.LogisticRegression`, `sklearn.linear_model.LogisticRegressionCV`,
+        `sklearn.svm.LinearSVC`, `sklearn.discriminant_analysis.LinearDiscriminantAnalysis`,
+        `sklearn.linear_model.RidgeClassifier`, `sklearn.linear_model.RidgeClassifierCV`,
+        `sklearn.linear_model.SGDClassifier`, or `Perceptron`.
+        If a pre-fitted classifier is provided, it is used to compute :math:`{\mathbf{Z}}`.
+        If None, ``sklearn.linear_model.LogisticRegression()``
+        is used as the classifier.


This is formatted weird in the docs, is there any other way we can do this?

Changed this to be a list for both KPCovC and PCovC (they have the similar docs here).

rosecers · 2025-06-24T22:42:40Z

src/skmatter/decomposition/_kernel_pcovr.py

@@ -76,8 +67,8 @@ class KernelPCovR(_BasePCA, LinearModel):
        If `precomputed`, we assume that the `y` passed to the `fit` function
        is the regressed form of the targets :math:`{\mathbf{\hat{Y}}}`.

-    kernel : "linear" | "poly" | "rbf" | "sigmoid" | "cosine" | "precomputed"
-        Kernel. Default="linear".
+    kernel : {'linear', 'poly', 'rbf', 'sigmoid', 'cosine', 'precomputed'} or callable, default='linear'


Why the formatting change here?

Would be fine but please use double quotes for strings.

I removed the vertical bars to match how we format the rest of the parameters that contain a list of options. Added double quotes

rosecers · 2025-06-24T22:44:44Z

examples/pcovc/KPCovC_Comparison.py

I LOVE this example.

PicoCentauri

Very nice. I have some minor styling comments but should be easy to implement.

CHANGELOG

examples/pcovc/KPCovC_Comparison.py

src/skmatter/decomposition/_kernel_pcovc.py

PicoCentauri · 2025-06-25T07:36:40Z

src/skmatter/decomposition/_kernel_pcovc.py

+        """
+        return super().inverse_transform(T)
+
+    def decision_function(self, X=None, T=None):


This function seems not tested very well. You think you can extend the tests to cover the points codecov is complaining?

Good point -- we are adding some more tests to this in our next update for multi-output PCovC/KPCovC. For now, I think I will leave it

Okay should this be doner in another PR? If yes just open an issue to keep track of the TODOs please.

@rvasav26 I think we should try to add these tests in this PR. These look easy to test.

PicoCentauri · 2025-06-25T07:37:49Z

src/skmatter/decomposition/_kernel_pcovr.py

@@ -76,8 +67,8 @@ class KernelPCovR(_BasePCA, LinearModel):
        If `precomputed`, we assume that the `y` passed to the `fit` function
        is the regressed form of the targets :math:`{\mathbf{\hat{Y}}}`.

-    kernel : "linear" | "poly" | "rbf" | "sigmoid" | "cosine" | "precomputed"
-        Kernel. Default="linear".
+    kernel : {'linear', 'poly', 'rbf', 'sigmoid', 'cosine', 'precomputed'} or callable, default='linear'


Would be fine but please use double quotes for strings.

src/skmatter/decomposition/_kpcov.py

tests/test_kernel_pcovr.py

PicoCentauri

Thanks for the changes. I am happy.

cajchristian · 2025-06-26T16:55:06Z

examples/pcovc/KPCovC_Comparison.py

+        "kernel_params": {"kernel": "rbf", "gamma": 12},
+        "title": "Logistic Regression",
+    },
+    RidgeClassifier(random_state=random_state): {


Could also use RidgeClassifierCV here!

cajchristian

I'm very happy - just left a few comments, and will approve once tests for the kpcovc decision function have been written.

rvasav26 added 17 commits June 17, 2025 14:15

Adding KPCovC to docs

888a37b

Changing assertTrue to assertEqual for correctness

4d537a1

Investigating into KPCovC inconsistencies

de226db

Trying out some things for KPCovC problems

6dada5d

Changing KPCovC's test_precomputed_classification

315f358

Continuing KPCovC investigation

e653076

Changing _BasePCov and _BaseKPCov to be abstract base classes

844c16e

Cleaning up print statements

3616619

Merging PCovC update

004499a

Removing KPCovC experiment

c676e10

Trying mixing=1.0 for KPCovC/PCovC match

7d04666

Switching KPCovC back to using SVC

115a224

Minor edits after cleaning up KPCovC branch

bed1b4b

Checking scaling and LinearSVC match

e93b86f

Working on docstrings

69dd1b6

Adding example drafts

b701e23

Switching from KPCovC w/SVC back to KPCovC w/linear classifiers

9e4e3d8

rvasav26 force-pushed the adding-kpcovc-new branch from 82aa115 to 9e4e3d8 Compare June 17, 2025 19:20

rvasav26 added 6 commits June 18, 2025 15:10

Finalizing examples

07aba83

Modifying tests

fe6b0c7

Modifying docstrings and minor edits

623bc1f

Updating CHANGELOG

7cef97c

Formatting

e0ecb03

More formatting and cleaning

26a246e

rvasav26 marked this pull request as ready for review June 19, 2025 17:16

rvasav26 requested review from PicoCentauri, rosecers and cajchristian June 19, 2025 17:16

Minor edits

15579d6

rosecers reviewed Jun 24, 2025

View reviewed changes

PicoCentauri reviewed Jun 25, 2025

View reviewed changes

rvasav26 added 3 commits June 25, 2025 17:27

CHANGELOG suggestion

993b215

Example suggestions

4962500

Docstring and other skmatter/decomposition suggestions

5e619ae

PicoCentauri approved these changes Jun 26, 2025

View reviewed changes

cajchristian reviewed Jun 26, 2025

View reviewed changes

Christian's suggestions and decision_function tests

a1a316a

rvasav26 force-pushed the adding-kpcovc-new branch from 714ea50 to a1a316a Compare June 26, 2025 21:14

cajchristian approved these changes Jun 27, 2025

View reviewed changes

PicoCentauri merged commit 004287c into main Jun 28, 2025
12 checks passed

PicoCentauri deleted the adding-kpcovc-new branch June 28, 2025 08:01

Adding Kernel PCovC Code #254

Adding Kernel PCovC Code #254

Uh oh!

Conversation

rvasav26 commented Jun 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contributor (creator of PR) checklist

For Reviewer

Uh oh!

rosecers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PicoCentauri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PicoCentauri Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

PicoCentauri left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cajchristian left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rvasav26 commented Jun 8, 2025 •

edited

Loading

PicoCentauri Jun 26, 2025 •

edited

Loading